fix(rpc): return correct error codes in upgradetohd rpc
#6797
Conversation
|
WalkthroughThe changes refactor the HD wallet upgrade process by removing the Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/wallet/rpc/wallet.cpp (1)
423-461: Consider exception-safe lock state managementThe wallet lock state preservation logic could leave the wallet in an incorrect state if an exception is thrown between unlocking (line 435) and re-locking (line 460). While the LOCK scope ensures thread safety, user expectations about wallet lock state might be violated.
Consider using RAII or scope guards to ensure the wallet is always restored to its original lock state, even in the presence of exceptions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/wallet/rpc/wallet.cpp(1 hunks)src/wallet/wallet.cpp(1 hunks)src/wallet/wallet.h(0 hunks)test/functional/wallet_upgradetohd.py(1 hunks)
💤 Files with no reviewable changes (1)
- src/wallet/wallet.h
🧰 Additional context used
📓 Path-based instructions (2)
test/functional/**/*.py
📄 CodeRabbit Inference Engine (CLAUDE.md)
Functional tests should be written in Python and placed in test/functional/
Files:
test/functional/wallet_upgradetohd.py
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit Inference Engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/wallet/rpc/wallet.cppsrc/wallet/wallet.cpp
🧠 Learnings (4)
📚 Learning: in the test framework consolidation pr (#6718), user kwvg prefers to limit functional changes to tho...
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Applied to files:
test/functional/wallet_upgradetohd.pysrc/wallet/rpc/wallet.cpp
📚 Learning: the `getwallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null chec...
Learnt from: kwvg
PR: dashpay/dash#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Applied to files:
test/functional/wallet_upgradetohd.pysrc/wallet/rpc/wallet.cppsrc/wallet/wallet.cpp
📚 Learning: pull request #6543 is focused on move-only changes and refactoring, specifically backporting from bi...
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Applied to files:
src/wallet/rpc/wallet.cppsrc/wallet/wallet.cpp
📚 Learning: the functions `migratedbifneeded` and `migratedbifneeded2` in `src/evo/deterministicmns.cpp` are tem...
Learnt from: knst
PR: dashpay/dash#6511
File: src/evo/deterministicmns.cpp:1369-1373
Timestamp: 2025-01-07T18:50:44.838Z
Learning: The functions `MigrateDBIfNeeded` and `MigrateDBIfNeeded2` in `src/evo/deterministicmns.cpp` are temporary and will be removed in a future version. Refactoring suggestions for these functions should be avoided.
Applied to files:
src/wallet/wallet.cpp
🧬 Code Graph Analysis (1)
test/functional/wallet_upgradetohd.py (2)
test/functional/test_framework/util.py (1)
assert_raises_rpc_error(132-148)src/wallet/rpc/wallet.cpp (2)
upgradetohd(345-492)upgradetohd(345-345)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: mac-build / Build source
- GitHub Check: linux64_tsan-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_fuzz-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64-build / Build source
- GitHub Check: win64-build / Build source
- GitHub Check: Lint / Run linters
🔇 Additional comments (4)
test/functional/wallet_upgradetohd.py (1)
206-206: LGTM! Test correctly updated to match fixed RPC error handling.This change properly aligns the test with the refactored
upgradetohdRPC implementation that now consistently returns error code -14 (RPC_WALLET_PASSPHRASE_INCORRECT) and the standardized error message for incorrect wallet passphrase scenarios. This ensures the test validates the corrected RPC behavior.src/wallet/wallet.cpp (1)
356-366: LGTM! Well-structured refactor of HD chain generation.The changes properly refactor the HD chain generation logic:
- Good defensive programming: Added proper null check for
GetLegacyScriptPubKeyMan()with appropriate error handling- Better encapsulation: Using
WithEncryptionKeycallback approach instead of direct method calls provides better separation of concerns and encryption key handling- Consistent error handling: Returns proper
DatabaseStatus::FAILED_ENCRYPTon failure- Clean lambda usage: The lambda properly captures variables and calls the script pubkey manager's
GenerateNewHDChainmethodThis aligns well with the PR objective of moving HD upgrade logic to the appropriate layer (script pubkey managers) and centralizing the functionality.
src/wallet/rpc/wallet.cpp (2)
375-468: Well-structured refactoring with improved error handlingThe refactoring successfully moves all HD upgrade logic into the RPC method with proper wallet locking, which is a significant improvement over the previous implementation. The new error handling provides consistent error codes for passphrase validation failures.
449-452: Error handling in WithEncryptionKey is correct; exceptions propagateThe HD-chain generator throws
std::runtime_erroron failure (e.g. inSetMnemonic), andCWallet::WithEncryptionKeymerely locks and invokes your callback without catching exceptions. Any exception fromGenerateNewHDChainwill bubble up, so returningtruein the callback only governs the boolean result when no error occurs. No changes are needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/wallet/rpc/wallet.cpp (1)
403-409: Track the v21 breaking changes TODOThe TODO comment indicates planned breaking changes for v21 where
upgradetohdwould be replaced withsethdseed. This technical debt should be tracked properly.Would you like me to create an issue to track this TODO for the v21 release?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/wallet/rpc/wallet.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/**/*.{cpp,h,cc,cxx,hpp}
📄 CodeRabbit Inference Engine (CLAUDE.md)
src/**/*.{cpp,h,cc,cxx,hpp}: Dash Core C++ codebase must be written in C++20 and require at least Clang 16 or GCC 11.1
Dash uses unordered_lru_cache for efficient caching with LRU eviction
Files:
src/wallet/rpc/wallet.cpp
🧠 Learnings (6)
📓 Common learnings
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Learnt from: kwvg
PR: dashpay/dash#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Learnt from: UdjinM6
PR: dashpay/dash#6786
File: ci/test/04_install.sh:99-101
Timestamp: 2025-08-01T07:46:37.840Z
Learning: In backport PRs like #6786, UdjinM6 prefers to defer non-critical fixes (such as shell command expansion issues) to separate commits/PRs to maintain focus on the primary backport objectives, consistent with the project's pattern of avoiding scope creep.
📚 Learning: the `getwallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null chec...
Learnt from: kwvg
PR: dashpay/dash#6529
File: src/wallet/rpcwallet.cpp:3002-3003
Timestamp: 2025-02-14T15:19:17.218Z
Learning: The `GetWallet()` function calls in `src/wallet/rpcwallet.cpp` are properly validated with null checks that throw appropriate RPC errors, making additional validation unnecessary.
Applied to files:
src/wallet/rpc/wallet.cpp
📚 Learning: pull request #6543 is focused on move-only changes and refactoring, specifically backporting from bi...
Learnt from: kwvg
PR: dashpay/dash#6543
File: src/wallet/receive.cpp:240-251
Timestamp: 2025-02-06T14:34:30.466Z
Learning: Pull request #6543 is focused on move-only changes and refactoring, specifically backporting from Bitcoin. Behavior changes should be proposed in separate PRs.
Applied to files:
src/wallet/rpc/wallet.cpp
📚 Learning: in pr #6761, kwvg acknowledged a null pointer check issue in chainlocksigner::cleanup() method but d...
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.cpp:247-250
Timestamp: 2025-07-29T14:32:48.369Z
Learning: In PR #6761, kwvg acknowledged a null pointer check issue in ChainLockSigner::Cleanup() method but deferred it to follow-up, consistent with the pattern of avoiding scope creep in refactoring PRs.
Applied to files:
src/wallet/rpc/wallet.cpp
📚 Learning: in the test framework consolidation pr (#6718), user kwvg prefers to limit functional changes to tho...
Learnt from: kwvg
PR: dashpay/dash#6718
File: test/functional/test_framework/test_framework.py:2102-2102
Timestamp: 2025-06-09T16:43:20.996Z
Learning: In the test framework consolidation PR (#6718), user kwvg prefers to limit functional changes to those directly related to MasternodeInfo, avoiding scope creep even for minor improvements like error handling consistency.
Applied to files:
src/wallet/rpc/wallet.cpp
📚 Learning: dash core uses bitcoin_ prefix for header guards as the standard convention, inherited from bitcoin ...
Learnt from: kwvg
PR: dashpay/dash#6761
File: src/chainlock/signing.h:5-6
Timestamp: 2025-07-23T09:30:34.631Z
Learning: Dash Core uses BITCOIN_ prefix for header guards as the standard convention, inherited from Bitcoin Core. Only a few BLS-specific files in src/bls/ use DASH_ prefix. The vast majority of files (385+) use BITCOIN_ prefix.
Applied to files:
src/wallet/rpc/wallet.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: x86_64-w64-mingw32 / Build depends
- GitHub Check: Lint / Run linters
- GitHub Check: x86_64-pc-linux-gnu_nowallet / Build depends
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
- GitHub Check: x86_64-pc-linux-gnu / Build depends
🔇 Additional comments (3)
src/wallet/rpc/wallet.cpp (3)
375-465: Well-structured refactoring that achieves the PR objectivesThe refactoring successfully moves the HD upgrade logic into the RPC method and ensures correct error codes are returned. The use of a scoped block for wallet locking and the proper handling of SecureString variables within the lock scope improves security.
423-436: Correct implementation of passphrase validationThe wallet locking and unlocking logic properly validates the passphrase and returns the correct error codes (
RPC_WALLET_PASSPHRASE_INCORRECT) as intended by this PR. This addresses the previous review feedback about ensuring the user knows the passphrase.
438-454: Proper delegation of HD chain generation to script pubkey managersThe refactored code correctly handles HD chain generation for both descriptor and legacy wallets, with appropriate handling of encrypted wallets using the
WithEncryptionKeycallback pattern. This clean separation of concerns improves the code architecture.
|
|
||
| // Unlock the wallet | ||
| if (!pwallet->Unlock(secureWalletPassphrase)) { | ||
| throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: The wallet passphrase entered was incorrect"); |
There was a problem hiding this comment.
once this PR (#6797) and (#6792) are meged, I would like to update error message like that:
if (secureWalletPassphrase.find('\0') == std::string::npos) {
throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: The wallet passphrase entered was incorrect.");
} else {
throw JSONRPCError(RPC_WALLET_PASSPHRASE_INCORRECT, "Error: The old wallet passphrase entered is incorrect. "
"It contains a null character (ie - a zero byte). "
"If the old passphrase was set with a version of this software prior to 23.0, "
"please try again with only the characters up to — but not including — "
"the first null character.");
…ng null character 960afcf feat: upgradetohd return string with futher instructions instead bool (Konstantin Akimov) 4f42cfa doc: add extra help for case if entered wallet passphrae is incorrect (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented Follow-up for #6797 and #6792 to update rpc error codes and add user's hint for `upgradetohd`. ## What was done? See commits. `upgradetohd` now returns a string instead bool (which is always true if no JSONRPCError is returned) which is better user experience. `upgradetohd` warns user if there's \0 in user's input of wallet_passphrase or mnemonic_passphrase. ## How Has This Been Tested? See updates in tests. ## Breaking Changes `upgradetohd` returns string instead bool. ## Checklist: - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 960afcf Tree-SHA512: 42351fbe84b824b04120a19814f4a24829bd2c8efab3079ff94d06af4a486d4c13f99d62ebe1e940f483e4151ddf54b8c47ffd31762c49bd0b6735524da9f6e8
3 participants
Issue being fixed or feature implemented
upgradetohdrpc returns wrong error codesWhat was done?
Move wallet "upgrade to HD" code used in rpc only to
upgradetohdrpc, return correct error codes. We also don't need all that logic to extract wallet master encryption key because it's already known when wallet is unlocked.How Has This Been Tested?
Run tests
Breaking Changes
upgradetohdshould return correct error codes now which could probably break things if you relied on them, see testsChecklist: